Add the ruff linter to the pre-commit check #214

SamRWest · 2024-03-12T00:51:33Z

I've been finding the ruff linter really useful in some other projects, so thought I'd see if you guys were interested in adding it here.

My motivation was mainly to get the imports formatting standardised (which black doesn't do) for easier diffing/merging.
But it's got a bunch of other really useful checks, especially when working with pandas (though I've turned these off for now so as not to confuse the PR diff).

It's also found and fixed a bunch of style/safety things and I've flagged a couple of logic errors to be fixed also.

What do you guys think?

reverted lambda change

SamRWest · 2024-03-12T01:05:00Z

pyproject.toml

+    "NPY", # numpy style
+    "PL", # pylint
+#    "PD", # pandas style # TODO enable later
+#    "C90", # code complexity # TODO enable later


these last two will require a reasonable number of (simple) changes, so I've left them off for now so this diff isn't too polluted.

Nice idea, thanks. Looking forward to the next linting PR. :)

SamRWest · 2024-03-12T01:05:39Z

tests/test_transforms.py

    _count_comm_group_vectorised,
+    _match_wildcards,
+    _process_comm_groups_vectorised,
+    commodity_map,


^ This was the original motivation - standard import sorting/formatting

SamRWest · 2024-03-12T01:06:55Z

utils/dd_to_csv.py

@@ -124,8 +123,8 @@ def parse_parameter_values_from_file(


 def save_data_with_headers(
-    param_data_dict: Dict[str, Union[pd.DataFrame, List[str]]],
-    headers_data: Dict[str, List[str]],
+    param_data_dict: dict[str, pd.DataFrame | list[str]],


What python versions are you planning to support?
This was done with the py11 setting (to match CI), but I'll revert and re-run for py39 if that's the minimum target. (I think the | type hint only works from 3.10 onwards)

We haven't had that discussion actually... How about py12?

Works for me, type hints are a lot cleaner in 3.12 for instance.
I just thought someone mentioned a use-case for running with an older version is all.

I think we use the match-case statement somewhere in transforms.py which is >= 3.11. I feel perhaps we should try to keep the min version as low as possible, just to make it easier for users? Not sure how much of a pain it is to install a specific Python version in Windows (in linux I use pyenv)

I'd say it is easy! :-) But I am also fine with sticking to 3.11.

Yeah I use pyenv on windows also, works very well.
Less savvy users can just download and run the official installers etc.
Ok, so we'll target >=3.11, cool.

SamRWest · 2024-03-12T01:08:28Z

utils/run_benchmarks.py

@@ -65,6 +65,7 @@ def run_gams_gdxdiff(
        stdout=subprocess.PIPE,
        stderr=subprocess.STDOUT,
        text=True,
+        check=False,


see https://docs.astral.sh/ruff/rules/subprocess-run-without-check/

olejandro · 2024-03-12T02:46:04Z

I generally like linters, but I don't have that much experience using them with python. I've used ESLint with javascript.

siddharth-krishna

Great idea, thanks! I've been thinking of adding a linter for a while, though I haven't used ruff myself, it looks neat (and fast!).

I still have an open branch somewhere to upgrade the version of pyright -- this should also lead to a bunch of pandas-related changes, perhaps ruff will help us fix many of those?

pyproject.toml

siddharth-krishna · 2024-03-12T06:40:47Z

pyproject.toml

+    "NPY", # numpy style
+    "PL", # pylint
+#    "PD", # pandas style # TODO enable later
+#    "C90", # code complexity # TODO enable later


Nice idea, thanks. Looking forward to the next linting PR. :)

utils/dd_to_csv.py

siddharth-krishna · 2024-03-12T06:45:32Z

utils/dd_to_csv.py

@@ -124,8 +123,8 @@ def parse_parameter_values_from_file(


 def save_data_with_headers(
-    param_data_dict: Dict[str, Union[pd.DataFrame, List[str]]],
-    headers_data: Dict[str, List[str]],
+    param_data_dict: dict[str, pd.DataFrame | list[str]],


I think we use the match-case statement somewhere in transforms.py which is >= 3.11. I feel perhaps we should try to keep the min version as low as possible, just to make it easier for users? Not sure how much of a pain it is to install a specific Python version in Windows (in linux I use pyenv)

utils/dd_to_csv.py

xl2times/utils.py

fixed loop lint errors

olejandro · 2024-03-13T22:27:40Z

@SamRWest I guess this one can be merged already? Just so we don't have to resolve too many conflicts at a later stage...

SamRWest · 2024-03-13T22:43:09Z

@SamRWest I guess this one can be merged already? Just so we don't have to resolve too many conflicts at a later stage...

Yep, I'll merge it shortly, then tackle #215

SamRWest added 3 commits March 12, 2024 11:30

added ruff and config

68f6e9d

Fixed ruff errors, ran fixes on all files.

7cfdbf2

set ruff to py39

04c16ae

reverted lambda change

SamRWest commented Mar 12, 2024

View reviewed changes

SamRWest requested review from siddharth-krishna and olejandro March 12, 2024 01:08

SamRWest marked this pull request as ready for review March 12, 2024 03:52

siddharth-krishna approved these changes Mar 12, 2024

View reviewed changes

siddharth-krishna changed the title ~~Samw/ruff linting~~ Add the ruff linter to the pre-commit check Mar 12, 2024

olejandro approved these changes Mar 12, 2024

View reviewed changes

ruff uses python>=3.11

f40e4a7

fixed loop lint errors

siddharth-krishna approved these changes Mar 13, 2024

View reviewed changes

Merge branch 'main' into samw/ruff_linting

dde7cb6

SamRWest merged commit 8acd004 into main Mar 13, 2024
1 check passed

SamRWest deleted the samw/ruff_linting branch March 13, 2024 22:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ruff linter to the pre-commit check #214

Add the ruff linter to the pre-commit check #214

SamRWest commented Mar 12, 2024 •

edited

Loading

SamRWest Mar 12, 2024

siddharth-krishna Mar 12, 2024

SamRWest Mar 12, 2024

SamRWest Mar 12, 2024

olejandro Mar 12, 2024

SamRWest Mar 12, 2024

siddharth-krishna Mar 12, 2024

olejandro Mar 13, 2024

SamRWest Mar 13, 2024

SamRWest Mar 12, 2024

olejandro commented Mar 12, 2024

siddharth-krishna left a comment

siddharth-krishna Mar 12, 2024

siddharth-krishna Mar 12, 2024

olejandro commented Mar 13, 2024 •

edited

Loading

SamRWest commented Mar 13, 2024

Add the ruff linter to the pre-commit check #214

Add the ruff linter to the pre-commit check #214

Conversation

SamRWest commented Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olejandro commented Mar 12, 2024

siddharth-krishna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olejandro commented Mar 13, 2024 • edited Loading

SamRWest commented Mar 13, 2024

SamRWest commented Mar 12, 2024 •

edited

Loading

olejandro commented Mar 13, 2024 •

edited

Loading